gguf: better type usage #655

ngxson · 2024-05-04T21:04:22Z

Follow up #640

Ref comments:

gguf: Add types for LLM architectures #640 (review) by @julien-c suggests using a check metadata["general.architecture"] === ... to select the correct type
gguf: Add types for LLM architectures #640 (comment) by @coyotte508 suggests using less generic but more verbose code

The type system introduce in this PR allows type-checking at both compile time & runtime:

const model: GGUFMetadata<GGUFType.STRICT> = null as any;

if (model["general.architecture"] === "whisper") {
	model["encoder.whisper.block_count"] = 0;
	// @ts-expect-error because it must be a number
	model["encoder.whisper.block_count"] = "abc";
}

if (model["tokenizer.ggml.model"] === undefined) {
	// @ts-expect-error because it's undefined
	model["tokenizer.ggml.eos_token_id"] = 1;
}
if (model["tokenizer.ggml.model"] === "gpt2") {
	// @ts-expect-error because it must be a number
	model["tokenizer.ggml.eos_token_id"] = undefined;
	model["tokenizer.ggml.eos_token_id"] = 1;
}

if (model["general.architecture"] === "mamba") {
	model["mamba.ssm.conv_kernel"] = 0;
	// @ts-expect-error because it must be a number
	model["mamba.ssm.conv_kernel"] = "abc";
}
if (model["general.architecture"] === "llama") {
	// @ts-expect-error llama does not have ssm.* keys
	model["mamba.ssm.conv_kernel"] = 0;
}

Type checks can be disable with GGUFMetadata<GGUFType.NON_STRICT>

ngxson · 2024-05-04T21:12:22Z

Also in the process, I noticed some details not being mentioned in gguf specs:

For generic fields, only general.architecture is required. Other fields are all optional.
LLM_KV_VOCAB_SIZE is not assured to present (if it's not present, we take the length of vocab list), ref:
https://github.com/ggerganov/llama.cpp/blob/842500144ee02c8b0a46d2cc43880f8d80998fa5/llama.cpp#L3779
Not a big problem for now, we can fix that later.
MOE is optional (as long as many other Rope and Attention related fields), ref:
https://github.com/ggerganov/llama.cpp/blob/842500144ee02c8b0a46d2cc43880f8d80998fa5/llama.cpp#L3785
(explain: third param of get_key is set to false, indicating that it is not required)
==> Resolved with 9a9e771
whisper.cpp indeed does not use gguf, ref code:
https://github.com/ggerganov/whisper.cpp/blob/58210d6a7634ea1e42e0a2dab611f4a0518731dc/whisper.cpp#L1258
So unfortunately we don't have any way to test whisper gguf. Regarding fields being mentioned in the documentation, I found a related discussion here: GGUF file format specification ggerganov/ggml#302 (comment)

packages/gguf/src/types.ts

mishig25 · 2024-05-06T08:18:26Z

Very nice PR! And much needed improvement to the typing of gguf, especially type inference.
Left some nits. Otherwise, we are close to merge 🚀

ngxson · 2024-05-06T10:05:07Z

packages/gguf/src/types.ts

-	Partial<Model> &
-	Record<string, MetadataValue>;
+} & GGUFModelKV &
+	(TGGUFType extends GGUFType.strict ? unknown : Record<string, MetadataValue>);


FYI, the unknown here allow casting between GGUFParseOutput<GGUFType.strict> and GGUFParseOutput<GGUFType.nonStrict>. Funny though, I have no idea why adding unknown make it work. Maybe this is not the best way to do, so feel free to tell if you have other suggestions.

packages/gguf/src/types.ts

julien-c

looks in good shape! do you want to take a quick look too @coyotte508?

mishig25 · 2024-05-06T14:16:59Z

Question regarding GGUFType.NON_STRICT. Why do we need GGUFType.NON_STRICT? Shouldn't we want GGUFType.STRICT to be always the case?

ngxson · 2024-05-06T15:11:56Z

I believe that GGUFType.NON_STRICT will be useful in case user want to use their own gguf structure. In fact, gguf can also be used for storing control vectors, lora weights, and maybe other use cases outside of llama.cpp

mishig25

LGTM !

mishig25 · 2024-05-07T09:11:30Z

@coyotte508 could you give a quick look and I will merge 🚀

coyotte508

Looks good!

I'm wondering about flat paths vs nested metadata (maybe it could be another option in the future 🤔) but LGTM with the 1/2 changes requested

coyotte508 · 2024-05-07T10:02:09Z

packages/gguf/src/gguf.ts

@@ -308,6 +308,9 @@ export async function gguf(
 			}
 		}
 		offset += valueResult.length;
+		/// TODO(fix typing)
+		// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+		// @ts-ignore


Let's use // @ts-expect-error instead of // @ts-ignore in general

(no need for eslint-disable this way)

Here I think you can change const metadata: GGUFMetadata to const metadata: GGUFMetadata<GGUFType.NON_STRICT> to remove the error (not sure if it's the best fix)

Fixed in 31bac8b

coyotte508 · 2024-05-07T10:08:36Z

packages/gguf/src/types.ts

+export enum GGUFType {
+	STRICT,
+	NON_STRICT,
+}
+
+export type GGUFMetadata<TGGUFType extends GGUFType = GGUFType.STRICT> = {


We should probably switch to something like https://github.com/sindresorhus/type-fest/blob/main/source/except.d.ts

interface GGUFMetadataOptions { /** * ... * * @default true */ strict: boolean; } export GGUFMetadata<Options extends GGUFMetadataOptions = { strict: true}> { ...

Good idea 👍 I've never thought about this before. Implemented in 31bac8b

ngxson · 2024-05-07T13:56:38Z

I'm wondering about flat paths vs nested metadata (maybe it could be another option in the future 🤔)

@coyotte508 I thought about using nested object, but that seems to be quite complicated atm, since part of the code/naming here is copied directly from cpp file (so it save us some headaches)

But I'm also agree that having nested object is a reasonable feature, since gguf keys are "namespaced" anyway. Maybe we will re-visit this idea later on.

julien-c · 2024-05-07T14:36:47Z

alright let's merge!

re. nested properties, that's what they did in https://github.com/ahoylabs/gguf.js (credited as one of the inspirations for this package) but I'm not sure it's worth it / I find it cleaner to just expose the raw data from the GGUF file.

Follow up #655 and #656 (comment) Added some examples on how to use local file + strictly typed --------- Co-authored-by: Julien Chaumond <[email protected]> Co-authored-by: Mishig <[email protected]>

refactor typing + usage

e061017

ngxson requested review from mishig25 and julien-c as code owners May 4, 2024 21:04

ngxson added 3 commits May 5, 2024 12:10

correct optional fields

9a9e771

add non-strict & strict typing mode

6d704bc

lint & format

8767726

mishig25 requested a review from coyotte508 May 6, 2024 08:06

mishig25 reviewed May 6, 2024

View reviewed changes

packages/gguf/src/types.ts Outdated Show resolved Hide resolved

mishig25 reviewed May 6, 2024

View reviewed changes

packages/gguf/src/types.ts Outdated Show resolved Hide resolved

cast between strict & nonStrict

c2afbdc

ngxson commented May 6, 2024

View reviewed changes

mishig25 reviewed May 6, 2024

View reviewed changes

packages/gguf/src/types.ts Outdated Show resolved Hide resolved

style nits

5f547dd

julien-c reviewed May 6, 2024

View reviewed changes

mishig25 approved these changes May 6, 2024

View reviewed changes

julien-c approved these changes May 7, 2024

View reviewed changes

coyotte508 reviewed May 7, 2024

View reviewed changes

ngxson added 5 commits May 7, 2024 15:15

refactor options

31bac8b

Merge branch 'main' into xsn/gguf_type_usage

74e8cfd

format

8c1bce0

fix CI

2e62e41

fix CI (2)

a2250d3

coyotte508 approved these changes May 7, 2024

View reviewed changes

julien-c merged commit 6a036d8 into huggingface:main May 7, 2024
3 of 4 checks passed

ngxson mentioned this pull request May 12, 2024

gguf: update README #663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gguf: better type usage #655

gguf: better type usage #655

ngxson commented May 4, 2024 •

edited

Loading

ngxson commented May 4, 2024 •

edited

Loading

mishig25 commented May 6, 2024

ngxson May 6, 2024

julien-c left a comment

mishig25 commented May 6, 2024

ngxson commented May 6, 2024 •

edited

Loading

mishig25 left a comment

mishig25 commented May 7, 2024

coyotte508 left a comment

coyotte508 May 7, 2024

ngxson May 7, 2024

coyotte508 May 7, 2024

ngxson May 7, 2024 •

edited

Loading

ngxson commented May 7, 2024 •

edited

Loading

julien-c commented May 7, 2024

gguf: better type usage #655

gguf: better type usage #655

Conversation

ngxson commented May 4, 2024 • edited Loading

ngxson commented May 4, 2024 • edited Loading

mishig25 commented May 6, 2024

ngxson May 6, 2024

Choose a reason for hiding this comment

julien-c left a comment

Choose a reason for hiding this comment

mishig25 commented May 6, 2024

ngxson commented May 6, 2024 • edited Loading

mishig25 left a comment

Choose a reason for hiding this comment

mishig25 commented May 7, 2024

coyotte508 left a comment

Choose a reason for hiding this comment

coyotte508 May 7, 2024

Choose a reason for hiding this comment

ngxson May 7, 2024

Choose a reason for hiding this comment

coyotte508 May 7, 2024

Choose a reason for hiding this comment

ngxson May 7, 2024 • edited Loading

Choose a reason for hiding this comment

ngxson commented May 7, 2024 • edited Loading

julien-c commented May 7, 2024

ngxson commented May 4, 2024 •

edited

Loading

ngxson commented May 4, 2024 •

edited

Loading

ngxson commented May 6, 2024 •

edited

Loading

ngxson May 7, 2024 •

edited

Loading

ngxson commented May 7, 2024 •

edited

Loading